feat(plugin): add Dameng DM8 support - #2114

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Review fixes pushed to this branch

I ran a review pass over the driver, the Rust bridge, the C ABI, and the core registry changes, then pushed 5 commits on top of 07c7d406d. Nothing was rebased or force pushed. Summary of what changed and why, so you can check my reasoning.

Data loss

Decode failures became SQL NULL. convert_cell collapsed every unreadable cell into TpDmCell::Null, which is indistinguishable from a real NULL. For a table with no primary key, rowPredicate builds the WHERE clause from every column, so that fake NULL became "col" IS NULL and the UPDATE or DELETE either hit a different row or silently matched nothing. Unreadable cells now come back as their raw bytes, so the save fails loudly instead of writing to the wrong row.

Binary DECIMAL decoded with the wrong value. decode_dm_binary_decimal computed the exponent into _exp and discarded it, dropped the sign, and capped the mantissa at i64. A DECIMAL(12,2) of 42.50 decoded as 4250, -1.23 decoded as 123, and anything wider than about 18 significant digits returned SQL NULL and fed straight into the predicate problem above.

The fix was mostly deletion of a duplicate: decode_dm_decimal_to_text in dameng-protocol/src/message/response.rs:455 already does this correctly and has four captured vectors pinning it. The dameng-types copy was a truncated fork of it. I also checked the algorithm against Dameng's own Go driver (dm8/o.go decodeDecimal) and it agrees byte for byte.

Two related things fell out of that. The call site's if s.is_ascii() { return None } was swallowing every negative binary decimal before the decoder ran, because a negative varnum is pure ASCII. And once that guard was removed, the decoder was permissive enough to turn text like 12.3.4 into a plausible number, so it now validates strictly against the vendor rule that a short negative value must carry the 0x66 terminator.

Packed DATE, TIME and TIMESTAMP rendered as control characters. The arm returned DmValue::Text for any payload that happened to be valid UTF-8, before any typed decode ran. A packed 10:08:45 is 0a 69 01 00 00, every byte below 0x80, so it took that path and reached the grid as mojibake. Decoding now dispatches on wire length the way the vendor driver does (DATE 3, TIME 5, TIME_TZ 7, DATETIME 8, DATETIME2 9, DATETIME_TZ 10, DATETIME2_TZ 11), and text is a validated fallback rather than a passthrough. Worth noting the comment above that code described the layout as hour, min, sec, nanos big-endian, which is not what DM sends. The 8 byte branch already in the file had it right.

View definitions were truncated then written back. fetchViewDefinition cast to VARCHAR(8188) with no truncation signal, and the driver also offers CREATE OR REPLACE VIEW, so opening a long view and saving replaced it with the truncated half. It now asks the server for LENGTHB(TEXT) and refuses rather than guessing from the returned length, which would have rejected a view of exactly 8188 bytes that is actually complete.

DROP TABLE was emitted for materialized views. dropObjectStatement mapped everything that was not VIEW to TABLE. TableOperationSQLBuilder.dropKeyword can pass MATERIALIZED VIEW, and DM8 supports them, so dropping one from the sidebar could destroy a same-named table. The default branch was the destructive one.

SQL injection

The driver interpolates literals rather than binding parameters, so DamengParameterBinder is the whole boundary.

DM8 supports a MySQL compatible mode where backslash escapes inside string literals. The binder only doubled quotes, so a trailing backslash escaped its own closing quote. It now probes the server once at connect with SELECT LENGTH('\\') FROM DUAL, escapes for the measured mode, and fails closed when it cannot measure. I avoided escaping backslash unconditionally because that would corrupt every Windows path and regex on a default mode server.

Two things I found while fixing that are worth calling out, because they are subtle:

• String.replacingOccurrences matches whole grapheme clusters, so a quote carrying a combining mark was never doubled. x' followed by U+0301 produced 'x'◌́ OR 1=1 -- ', terminating the literal and executing the rest. Escaping now walks Unicode scalars. This affected the original quote doubling too, not only the new backslash handling.
• The binder is not the only literal path. The foreign key preview, row actions and exports call escapeStringLiteral, which was inheriting the PluginKit default that only doubles quotes. DamengPluginDriver now overrides it and requiresBackslashEscapingInLiterals.

Row caps

applyingRowCap wrapped user SQL in SELECT * FROM (...) TABLEPRO_ROW_CAP WHERE ROWNUM <= n. Two problems: an inline view rejects duplicate output column names, so any join where both sides have an ID failed once a cap was active, and a WITH clause is not legal at the start of an inline view, so every CTE failed.

I deleted the rewrite rather than patching it, because the cap is already enforced natively. query_result fetches row_cap + 1, sets is_truncated, and truncates to row_cap, and executeBound was already passing rowCap down alongside the rewrite. Deleting it keeps the cap, fixes joins and CTEs, and removes the bespoke lexer. It also stops the rewrite being applied to the driver's own metadata queries.

While there, rowCap of nil reached the bridge as 0 and took an unbounded branch, which is the only driver in Plugins/ with no ceiling. It now clamps to PluginRowLimits.emergencyMax like every peer, and a query that hits that ceiling with no explicit cap raises an error rather than silently truncating an export.

Connection lifecycle

connect() returned success without contacting the server whenever rawConnection was non nil, so after a protocol error killed the client the connection could never be revived and every later call reported "Dameng connection is closed". disconnect() used queue.sync on the same serial queue that blocks for a full network round trip, so closing during a slow query blocked the caller, which on the main actor is a visible hang. Handle state is now behind a lock with an explicit shutdown flag, and teardown is asynchronous.

Also: connect() treated the V$VERSION banner query as fatal. That view is DBA restricted, so a normal application user could not connect at all. It is best effort now, matching OraclePlugin. The live suite only ever runs as SYSDBA, which is why this was not caught.

Explain plan

The registry entry omitted format: .damengText and no .dameng case existed in ExplainPlanFormatDefaults, so the resolver fell through to plain text and DamengPlanParser was never selected. Both are fixed.

The plan parser also derived tree depth from the spaces between the line number and #. DM8 column aligns that marker, so the gap shrinks as the line number widens and the tree reparents. It measures the marker column now. For the record this needs 100+ plan lines to trigger, not 10, because the collision only happens once line numbers reach three digits.

CI

cargo test was not run anywhere, so the bridge's own test had never executed. I added it, and made it cover the vendored crates too. They are path dependencies rather than workspace members, so testing the bridge manifest alone skipped roughly 195 decoder tests. That is how the decimal and datetime bugs shipped.

Verification

• cargo test across all four crates: 201 passed
• swiftlint lint --strict repo wide: 0 violations in 1424 files
• AllPlugins and TablePro builds: succeeded
• TableProTests targeted suites: 110 passed
• DamengDriverTests: 9 tests, 0 failures, 2 skipped
• scripts/check-pluginkit-abi.sh 07c7d406d: unchanged

Each new test was checked to fail on the previous code, not just to pass on the new code.

Two things I could not verify, and one for you

I had no DM8 server, so the 2 integration cases skipped throughout.

1. The backslash mode probe is unverified against a real server. Please run SELECT LENGTH('\\') FROM DUAL on your instance. It should return 2 in the default mode and 1 in MySQL compatible mode. That single result decides whether the escaper picks the right branch.
2. The packed date layout is corroborated but not captured. It matches the vendor Go driver, the JDBC dispatch, and the 8 byte branch already in this PR, but there are no captured DM8 bytes in the repo. This settles it in one query:

   SELECT CAST('2024-06-15' AS DATE), CAST('10:30:45.123456' AS TIME),
          CAST('2024-06-15 10:30:45.123456' AS TIMESTAMP) FROM DUAL

   Lengths of 3, 5 and 8 confirm the whole model. If it is wrong, cells render as hex rather than as a wrong date, which is the safe direction.
3. TIMESTAMP WITH TIME ZONE offsets are validated but discarded, so those values lose their offset. That is a gap rather than corruption, but it should get a changelog line if it ships this way.

I added the abi-additive label, since the ExplainPlanFormat.damengText addition is additive and the gate needs it.

Happy to talk through any of these if you disagree with a call, particularly deleting applyingRowCap and making the version query best effort.